feat(logger): middy middleware #313

saragerion · 2021-12-14T21:28:54Z

Description of your changes

Added:

Middy middleware functionality, unit tests, example, documentation
Small update to the cold start detection business logic, bug captured during the middleware unit test implementation
Removed dummy "deadbeef" references in dummy events, improved consistency of values in test events
Fixed linting issues in the logger folder
Fixed issue with github workflows

How to verify this change

Build the doc with npm run docs-runLocalDocker
Go to http://localhost:8000/core/logger/

Related issues, RFCs

#294

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

Breaking change checklist

N/A

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

… in test events

saragerion · 2021-12-16T00:23:24Z

docs/core/logger.md

@@ -116,7 +134,7 @@ In both case, the printed log will look like this:
        "cold_start": true,
        "function_arn": "arn:aws:lambda:eu-central-1:123456789012:function:shopping-cart-api-lambda-prod-eu-central-1",
        "function_memory_size": 128,
-        "function_request_id": "c6af9ac6-7b61-11e6-9a41-93e8deadbeef",
+        "function_request_id": "c6af9ac6-7b61-11e6-9a41-93e812345678",


Small change I did in this PR: removed all these weird "deadbeef" references

saragerion · 2021-12-16T00:23:58Z

packages/logger/README.md


 ```typescript
 // Environment variables set for the Lambda
 process.env.LOG_LEVEL = 'INFO';
 process.env.POWERTOOLS_SERVICE_NAME = 'hello-world';
+import { injectLambdaContext } from '../src/middleware/middy';


I will update the relative paths of the examples in the readme file in another PR

saragerion · 2021-12-16T00:25:24Z

packages/logger/examples/inject-context-decorator.ts

@@ -7,7 +7,7 @@ process.env.POWERTOOLS_SERVICE_NAME = 'hello-world';

 import * as dummyEvent from '../../../tests/resources/events/custom/hello-world.json';
 import { context as dummyContext } from '../../../tests/resources/contexts/hello-world';
-import { LambdaInterface } from './utils/lambda/LambdaInterface';
+import { LambdaInterface } from '../types/LambdaInterface';


@flochaz the examples folder referenced the types because of this one.
I move the LambdaInterface inside the types

saragerion · 2021-12-16T00:28:19Z

packages/logger/tsconfig.json

@@ -13,7 +13,8 @@
        "resolveJsonModule": true,
        "pretty": true,
        "baseUrl": "src/",
-        "rootDirs": [ "src/" ]
+        "rootDirs": [ "src/" ],
+        "esModuleInterop": true


Related: middyjs/middy#203 (comment)

saragerion · 2021-12-16T00:28:59Z

tests/resources/events/aws/apigateway-aws-proxy.json

@@ -32,7 +32,7 @@
    "Host": "1234567890.execute-api.eu-central-1.amazonaws.com",
    "Upgrade-Insecure-Requests": "1",
    "User-Agent": "Custom User Agent String",
-    "Via": "1.1 08f323deadbeefa7af34d5feb414ce27.cloudfront.net (CloudFront)",
+    "Via": "1.1 08f32312345678a7af34d5feb414ce27.cloudfront.net (CloudFront)",


This is again the "deadbeef" reference I replaced, in case you wonder

dreamorosi

Great job and thanks for the heavy lifting with the first middleware implementation!

saragerion · 2021-12-16T11:12:05Z

.github/workflows/on-push.yml

@@ -16,7 +16,7 @@ jobs:
          node-version: '14'
      - name: Install packages
        run: |
-          npm ci
+          npm install


Fixed this: https://github.com/awslabs/aws-lambda-powertools-typescript/runs/4542223081?check_suite_focus=true

Oh nice catch

flochaz · 2021-12-16T13:14:53Z

docs/core/logger.md

    import { Logger } from "@aws-lambda-powertools/logger";
+    import { injectLambdaContext } from '@aws-lambda-powertools/logger/middleware/middy';


Should we expose it at Top level, such as import { Middleware } from '@aws-lambda-powertools/logger'; ?

flochaz · 2021-12-16T13:15:07Z

docs/core/logger.md

+        logger.info("This is an INFO log with some context");
+    };
+
+    const handler = middy(lambdaHandler)


middy not defined

flochaz · 2021-12-16T13:18:58Z

packages/logger/README.md

@@ -91,6 +92,9 @@ const lambdaHandler: Handler = async () => {

 };

+const handlerWithMiddleware = middy(lambdaHandler)
+  .use(injectLambdaContext(logger));


Not sure of the naming here, can't we get it from logger logger.Middleware. injectLambdaContext() instead ? otherwise how do I know it's a logger middleware ?

Interesting observation. So here is my reasoning:

With decorators being bound to a class in TypeScript, I thought it was intuitive to make it a method of the Logger class. That's not the case of a middleware.

The expected Lambda middleware's DX is normally in the form of functional programming, see an example of typical middleware: https://github.com/middyjs/middy/blob/main/packages/http-cors/index.js

That being said, your observation still stands. How do I know it's a logger middleware? For now, through the explicit import from the Logger package, a mix IDE tooltips, the explicit type of the middleware's parameters (target: Logger | Logger []).

In the future, to remove code duplication, I could see this middleware being moved in the commons package and becoming "package agnostic": the middleware takes a logger instance, or a metrics instance, and so on, any instance that implements a common interface with the addContext method.
What do you think?

sounds good !

flochaz · 2021-12-16T13:19:58Z

packages/logger/examples/inject-context-decorator.ts

@@ -7,7 +7,7 @@ process.env.POWERTOOLS_SERVICE_NAME = 'hello-world';

 import * as dummyEvent from '../../../tests/resources/events/custom/hello-world.json';
 import { context as dummyContext } from '../../../tests/resources/contexts/hello-world';
-import { LambdaInterface } from './utils/lambda/LambdaInterface';
+import { LambdaInterface } from '../types/LambdaInterface';


flochaz · 2021-12-17T06:22:17Z

packages/logger/README.md

@@ -91,6 +92,9 @@ const lambdaHandler: Handler = async () => {

 };

+const handlerWithMiddleware = middy(lambdaHandler)
+  .use(injectLambdaContext(logger));


sounds good !

feat(logger): middy middleware

1e1fcbc

dreamorosi assigned saragerion Dec 15, 2021

dreamorosi added enhancement logger This item relates to the Logger Utility labels Dec 15, 2021

dreamorosi added this to the beta-release milestone Dec 15, 2021

saragerion added 3 commits December 16, 2021 01:18

chore(all): remove deadbeef references, improve consistency of values…

63eb569

… in test events

feat(logger): middleware, cold start business logic + tests

8b6e2bb

docs(logger): middleware

7a75988

saragerion commented Dec 16, 2021

View reviewed changes

saragerion added 3 commits December 16, 2021 02:52

chore(logger): fix linting

b3154be

chore(logger): generate npm-shrinkwrap.json

0fbbccc

build: update on-push, on-release to fix eslint error

abfcf43

saragerion requested review from flochaz, dreamorosi and ijemmy December 16, 2021 02:04

saragerion linked an issue Dec 16, 2021 that may be closed by this pull request

Feature request: logger middy middleware support #294

Closed

saragerion marked this pull request as ready for review December 16, 2021 02:11

saragerion mentioned this pull request Dec 16, 2021

chore(commons): Create a common package #314

Merged

10 tasks

dreamorosi previously approved these changes Dec 16, 2021

View reviewed changes

saragerion commented Dec 16, 2021

View reviewed changes

flochaz suggested changes Dec 16, 2021

View reviewed changes

fix: middleware, middy import, examples, on-push, on-pr pipelines

497cc3d

saragerion dismissed dreamorosi’s stale review via 497cc3d December 16, 2021 20:16

saragerion added 3 commits December 16, 2021 22:01

chore: merge conflict

5cfdc42

fix: post-merge fixes

fcb4a90

chore: linting fix

db6ad64

saragerion added 3 commits December 16, 2021 22:21

chore: add npm-shrinkwrap.jso

d91f6aa

chore: update npm-shrinkwrap.json

212b0d1

fix(logger): lint PowertoolLog

9f7fdcd

flochaz approved these changes Dec 17, 2021

View reviewed changes

dreamorosi self-requested a review December 17, 2021 10:39

dreamorosi approved these changes Dec 17, 2021

View reviewed changes

saragerion merged commit 1b92a1e into main Dec 17, 2021

saragerion deleted the feat/logger-middleware branch December 17, 2021 10:50

dreamorosi mentioned this pull request Dec 17, 2021

Feature request: main implementation, randomization, middy middleware, correlation ID's, optional: object merging #139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logger): middy middleware #313

feat(logger): middy middleware #313

saragerion commented Dec 14, 2021 •

edited

Loading

saragerion Dec 16, 2021

saragerion Dec 16, 2021

saragerion Dec 16, 2021

flochaz Dec 16, 2021

saragerion Dec 16, 2021

saragerion Dec 16, 2021

dreamorosi left a comment

saragerion Dec 16, 2021

dreamorosi Dec 16, 2021

flochaz Dec 16, 2021

flochaz Dec 16, 2021

flochaz Dec 16, 2021

saragerion Dec 16, 2021 •

edited

Loading

flochaz Dec 17, 2021

flochaz Dec 16, 2021

flochaz Dec 17, 2021

		import { Logger } from "@aws-lambda-powertools/logger";
		import { injectLambdaContext } from '@aws-lambda-powertools/logger/middleware/middy';

feat(logger): middy middleware #313

feat(logger): middy middleware #313

Conversation

saragerion commented Dec 14, 2021 • edited Loading

Description of your changes

How to verify this change

Related issues, RFCs

PR status

Checklist

Breaking change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamorosi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saragerion Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saragerion commented Dec 14, 2021 •

edited

Loading

saragerion Dec 16, 2021 •

edited

Loading